Skip to content

Conversation

@Ryanseanlee
Copy link
Contributor

Hoist P/R Checklist

Pull request authors: Review and check off the below. Items that do not apply can also be
checked off to indicate they have been considered. If unclear if a step is relevant, please leave
unchecked and note in comments.

  • Caught up with develop branch as of last change.
  • Added CHANGELOG entry, or determined not required.
  • Reviewed for breaking changes, added breaking-change label + CHANGELOG if so.
  • Updated doc comments / prop-types, or determined not required.
  • Reviewed and tested on Mobile, or determined not required.
  • Created Toolbox branch / PR, or determined not required.

If your change is still a WIP, please use the "Create draft pull request" option in the split
button below to indicate it is not ready yet for a final review.

Pull request reviewers: when merging this P/R, please consider using a squash commit to
collapse multiple intermediate commits into a single commit representing the overall feature
change. This helps keep the commit log clean and easy to scan across releases. PRs containing a
single commit should be rebased when possible.

@Ryanseanlee Ryanseanlee requested a review from RyanMG October 20, 2025 20:17
RyanMG and others added 3 commits October 20, 2025 16:32
…cted.

 - update to code mirrors own CSS now uses !important for the same class
    with a relative position
@amcclain amcclain changed the title Codemirror6&json schema Upgrade to CodeMirror v6 + add JSON Schema support Oct 24, 2025
@amcclain amcclain changed the title Upgrade to CodeMirror v6 + add JSON Schema support CodeMirror v6 + JSON Schema support Oct 24, 2025
@Ryanseanlee Ryanseanlee marked this pull request as ready for review November 3, 2025 23:28
Copy link
Contributor

@PeteDarinzo PeteDarinzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed with comment

@amcclain
Copy link
Member

@PeteDarinzo thanks for looking at these with Ryan. Does this look complete and mergable from your POV? If so could you please approve the PR to indicate as much, or if any concerns or more testing / closer look needed don't hesitate to say as much. Thanks much would love to get it in (safely) when ready.

@PeteDarinzo
Copy link
Contributor

@amcclain PR is nearly ready, but I almost overlooked the legacy editorProps prop. The new version does not support this prop, and it is used in at least two client apps, in one case to turn off line numbers, in the other to provide extra keyboard mappings. I would assume the use of this prop is quite narrow, but I will conduct a poll of usage across other client apps. The prop itself might not need continued support, and if its current use is limited it may be practical to add props to support whatever it was used for (toggling line numbers, providing keymaps).

@amcclain
Copy link
Member

if its current use is limited it may be practical to add props to support whatever it was used for (toggling line numbers, providing keymaps).

Great, fully agree with that. In general it's nice to have an "escape hatch" that allows us to set any underlying props that the library component might support - is that general concept not supportable with the upgraded version? Doesn't need to be a blocker but would be interested to understand better

Copy link
Contributor

@PeteDarinzo PeteDarinzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved

@lbwexler
Copy link
Member

lbwexler commented Jan 6, 2026

Looks like the line number/tree gutter of json input gets substantially wider -- I think its a regression. Can we take a look?
(the tree affordances are now on the left of the line numbers -- which seems better, but the width growth is a problem)

} = this.componentProps,
extensions = [
// Theme
this.themeCompartment.of(this.getThemeExtension()),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you build the themeCompartment.of in to getThemeExtension(), so this looks like all the other extensions

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes but note changes to line 301, 498, and 501-503. getThemeExtension will now create the theme extension Compartment and the reaction calls getTheme() to reconfigure.

@Ryanseanlee
Copy link
Contributor Author

Looks like the line number/tree gutter of json input gets substantially wider -- I think its a regression. Can we take a look? (the tree affordances are now on the left of the line numbers -- which seems better, but the width growth is a problem)

I reordered the lint gutter, number gutter and fold gutter to match previous CM5. The widths are actually the same I think the order is what made it look wider as there was alot of empty space to the left of the number.

# Conflicts:
#	CHANGELOG.md
#	desktop/cmp/input/CodeInput.scss
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants